Move to Go 1.26.8 and clear reachable dependency vulnerabilities - #394
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughThe PR standardizes Go 1.26.5 across modules, Docker builders, CI, E2E, and release workflows. It updates indirect Go dependencies, upgrades ECharts to 6.1.0, refreshes the pinned release action, and documents the dependency changes. ChangesToolchain and module updates
CI and release alignment
Client dependency and changelog updates
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This dependency and toolchain refresh addresses known vulnerabilities without changing production code. Reported validation is successful, so the change is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/go.mod`:
- Line 3: Add pkg/** to the path filters for the lint, vet, and test jobs in
ci-server.yml, ci-alerter.yml, and ci-collector.yml, matching the service image
build behavior in ci-docker.yml.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55d8e3be-c0c3-4cf2-a662-c30fe3c6ec89
⛔ Files ignored due to path filters (4)
alerter/src/go.sumis excluded by!**/*.sumclient/package-lock.jsonis excluded by!**/package-lock.jsoncollector/go.sumis excluded by!**/*.sumserver/src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.github/workflows/ci-alerter.yml.github/workflows/ci-collector.yml.github/workflows/ci-e2e.yml.github/workflows/ci-server.yml.github/workflows/release.ymlalerter/Dockerfilealerter/src/go.modclient/package.jsoncollector/Dockerfilecollector/go.moddocs/changelog.mdpkg/go.modserver/Dockerfileserver/src/go.mod
12181e6 to
1d63282
Compare
AntTheLimey
left a comment
AntTheLimey
left a comment
There was a problem hiding this comment.
The changelog line "The client now reports no known vulnerabilities"
doesn't hold up against a fresh npm audit: it currently reports 7
(5 moderate, 2 high), down from 11 on main, all inside the vitest
dev-dependency tree and therefore not shipped in the built client, but
the sentence claims zero, and the same entry credits refreshing
js-yaml while js-yaml is still one of the two high findings. The
entry is also wrong the other way: it names four cleared
vulnerabilities, where govulncheck on main reports 14 reachable in
server, 14 in alerter, 9 in pkg and 7 in collector, all cleared by
this same bump. The fix is better than advertised. One edit fixes
both: say what was measured, so a later CVE audit can trust the
enumeration.
Two non-blocking notes: the title still says Go 1.26.5, but the
follow-up commit moved every pinned location (workflows, Dockerfiles,
all four go.mod files) to 1.26.8 consistently, so it's just the title
that's stale. And CodeRabbit's comment on pkg/go.mod is right that
ci-server.yml, ci-alerter.yml, and ci-collector.yml don't watch
pkg/** the way ci-docker.yml does, so a future pkg-only change would
rebuild and publish service images without running their lint/vet/test
jobs; this PR happens to dodge that because every commit here also
touches each service's own go.mod.
Everything else checks out against measurement: govulncheck reports
zero reachable vulnerabilities in all four Go modules on this branch,
versus 7/14/14/9 reachable stdlib issues on main, all fixed by the
go1.26.8 pin; all three service modules build cleanly against the
local pkg replace; gofmt and golangci-lint are clean everywhere;
npm ci and npm run lint pass on the client; and collector coverage
measures the same 85.9% on main and on this branch, so the four
failing collector CI jobs are the inherited COVERAGE_MIN=86 ratchet,
not a regression from this change.
No conflicts against #419 or #420 (merge-tree resolves cleanly against
both); the only shared file is docs/changelog.md, and neither sibling
touches a module file, Dockerfile, or workflow this PR changes.
Codacy reports a large number of SCA advisories against this repository, but most of them are not reachable from Workbench code, so I ran govulncheck across all four Go modules to find the ones that actually are. Four came back, and this change clears all of them. Three are standard library issues fixed in the 1.26.4 and 1.26.5 patch releases: a privacy leak in the crypto/tls Encrypted Client Hello handling, arbitrary input included unescaped in net/textproto error messages, and inefficient candidate hostname parsing in crypto/x509. The fourth is an infinite loop in golang.org/x/text normalisation, reached through pgxpool connection setup in all three services and directly from the server's password dictionary handling. The Go version is raised in the four module files, in the collector, server and alerter Dockerfile builder stages, and in the CI and release workflows. The release workflow is the important one, because it previously built the published binaries with the affected toolchain, so the advisories would have shipped regardless of what the module files said. Where a workflow repeats the version in an `if:` guard, the guard moved with the matrix value; leaving those behind would have silently stopped the coverage and publish steps from matching. golang.org/x/text goes to v0.40.0, which is the current release rather than the v0.39.0 floor govulncheck reports. go mod tidy also advanced golang.org/x/sync to v0.22.0 as a resolution side effect. On the client, ECharts moves to v6.1.0 to resolve a cross-site scripting advisory, and postcss, js-yaml and brace-expansion are refreshed transitively; npm audit now reports no known vulnerabilities. govulncheck reports zero affected symbols in all four modules after this change, down from three, three, four and four. The remaining advisories it lists are golang.org/x/crypto/ssh and ssh/agent issues that no Workbench code calls. No production code changes, so there is no new test coverage to add.
Trivy flags go 1.26.5 against seven stdlib CVEs fixed in 1.26.6, so the toolchain bump this branch made has been overtaken. Go to 1.26.8, the current 1.26.x patch, rather than the bare 1.26.6 minimum.
ci-docker.yml already rebuilds the service images when the shared pkg module changes, but ci-server.yml, ci-alerter.yml and ci-collector.yml only watched their own directories, so a pkg-only change could publish images without running the services' lint, vet and test jobs. Add pkg/** to the push and pull_request path filters of all three.
The changelog entry claimed four cleared vulnerabilities and a clean client audit; neither matched a fresh measurement. Restate it with the govulncheck figures actually measured on the previous toolchain (ten reachable advisories in the server, ten in the alerter, seven in pkg and six in the collector, all cleared by this change) and the npm audit figures before and after. Move the vitest development dependencies to v4.1.11, which is a patch release within the existing major, and refresh the transitive postcss, js-yaml, nanoid, brace-expansion, fflate and vite packages so that npm audit reports no findings. None of these ship in the built client.
dpage
changed the title
1d63282 to
dc91a81
Compare
|
Thanks, all three points were right. I've rebased onto main and rewritten the changelog entry around what govulncheck and npm audit actually report: on main it finds 10 reachable advisories in the server, 10 in the alerter, 7 in pkg and 6 in the collector (govulncheck 1.6.0, scanned with a 1.26.3 toolchain, which I suspect is why my numbers sit below yours), and zero in all four on this branch. On the client I bumped the vitest packages to 4.1.11 (a patch release within the same major) and let |
2 participants
Background
Codacy currently reports a large pile of SCA advisories against this
repository, and most of them are noise in the sense that nothing in the
Workbench actually calls the vulnerable code. Rather than bump things on
the strength of the advisory list, I ran
govulncheckacross all four Gomodules to establish which vulnerabilities are genuinely reachable from
our own call graphs. Four came back, and this change clears all four.
golang.org/x/textv0.33.0pgxpoolsetup in all three services, andserver/src/internal/auth/common_passwords.go:105crypto/tls(ECH privacy leak)net/textproto(unescaped input in errors)pkg,server,alertercrypto/x509(hostname parsing DoS)What changed
The Go version moves to 1.26.5 in the four module files, the three
service
Dockerfilebuilder stages, and the CI and release workflows.The release workflow is the one that actually mattered: it built the
published binaries with
1.26.2, so the standard library advisorieswould have shipped in released artefacts no matter what the module files
said. Codacy was in fact resolving some findings against that pin rather
than against
go.mod(affectedVersion: v1.26.2).Where a workflow repeats the Go version inside an
if:guard, the guardmoved along with the matrix value. Bumping only the matrix would have
left those conditions unmatchable and silently stopped the coverage and
publish steps from running, which is a quieter failure than a red build.
golang.org/x/textgoes to v0.40.0, the current release, rather thanstopping at the v0.39.0 floor
govulncheckreports.go mod tidyalsoadvanced
golang.org/x/syncto v0.22.0 as a resolution side effect.On the client, ECharts moves to v6.1.0 for a cross-site scripting
advisory, and
postcss,js-yamlandbrace-expansionare refreshedtransitively via
npm audit fix.npm auditnow reports no knownvulnerabilities, where it previously reported one moderate and four high.
Verification
govulncheckreports zero affected symbols in all four modules,down from three, three, four and four respectively. What it still
lists are
golang.org/x/crypto/sshandssh/agentadvisories that noWorkbench code calls.
pkg,collectorandalertertestsuites pass in full.
build succeeds, and 3,488 tests across 171 files pass.
Two things a reviewer should know rather than discover:
server/internal/toolshas two failing tests,TestStoreMemoryGeneratesEmbeddingIntegrationandTestRecallMemoriesGeneratesQueryEmbeddingIntegration, both failingwith
expected 3 dimensions, not 4000. This is the pre-existingvector(3)fixture problem from Gemini provider: (1) knowledge base search silently falls back to OpenAI due to missing gemini_embedding column in search_knowledgebase.go; (2) session startup fails with 400 "empty Part" error #337 and is not caused by thischange; I confirmed it reproduces identically on unmodified
mainwith the previous toolchain. It does not surface in CI because the CI
Postgres lacks pgvector, so these tests skip there.
gofmtflags sixpkg/**_test.gofiles, but it does so identicallyon
mainand under the old toolchain: those files use the project'sfour-space indentation, which collides with gofmt's tabs. I have left
them alone rather than fold a large unrelated reformat into a security
change.
No production code is modified, so there is no new coverage to add.
Related
Separately from this PR, I triaged the 40 code-level Codacy security
findings (SQL injection, hardcoded secrets, cookie flags, timing
attacks) and ignored them with recorded justifications; all 40 were
false positives in test code, with none in production code. Open
security items went from 155 to 115, and the 115 that remain are the
dependency advisories this PR addresses.
Summary by CodeRabbit
Security
Maintenance
Documentation